Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine routes list functionality #58

Open
wants to merge 25 commits into
base: 2.10.x
Choose a base branch
from

Conversation

settermjd
Copy link
Contributor

Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes

Description

This PR implements all but one of the suggestions that @gsteel made in #52. Thank you for the motivation and inspiration to make the changes and improve the original work.

@settermjd settermjd requested a review from gsteel November 14, 2024 11:55
@settermjd settermjd self-assigned this Nov 14, 2024
@settermjd settermjd force-pushed the refine-routes-list-functionality branch 2 times, most recently from 49a82a8 to 8629833 Compare November 14, 2024 11:58
@settermjd
Copy link
Contributor Author

settermjd commented Nov 14, 2024

I don't know if I've messed this PR up a little, given the number of commits. Also, the Psalm results I have locally are different from the GitHub check results. Would love some help getting them in sync.

@gsteel gsteel mentioned this pull request Nov 17, 2024
@alexmerlin
Copy link
Member

I don't know if I've messed this PR up a little, given the number of commits. Also, the Psalm results I have locally are different from the GitHub check results. Would love some help getting them in sync.

@settermjd Try running Psalm locally without cache:

vendor/bin/psalm --no-cache

Or explicitly clear cache first:

vendor/bin/psalm --clear-cache

before running Psalm as usually.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @settermjd - I've got quite a lot of feedback here 😬

Overall, I think a lot of this can be simplified.

src/ConfigProvider.php Outdated Show resolved Hide resolved
src/Routes/Filter/RouteFilterOptions.php Outdated Show resolved Hide resolved
src/Routes/Filter/RouteFilterOptions.php Outdated Show resolved Hide resolved
src/Routes/Filter/RouteFilterOptionsInterface.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Routes/ListRoutesCommand.php Outdated Show resolved Hide resolved
src/Routes/Filter/EmptyRouteFilterOptions.php Outdated Show resolved Hide resolved
test/Routes/ListRoutesCommandTest.php Outdated Show resolved Hide resolved
src/Routes/Filter/RouteFilterOptions.php Outdated Show resolved Hide resolved
@gsteel gsteel mentioned this pull request Nov 26, 2024
@settermjd
Copy link
Contributor Author

Sorry @settermjd - I've got quite a lot of feedback here 😬

Overall, I think a lot of this can be simplified.

Your feedback is always appreciated.

settermjd added a commit to settermjd/mezzio-tooling that referenced this pull request Dec 10, 2024
settermjd added a commit to settermjd/mezzio-tooling that referenced this pull request Dec 10, 2024
Thanks to @froschdesign for pointing this out in
mezzio#58 (comment).

Signed-off-by: Matthew Setter <[email protected]>
@settermjd settermjd force-pushed the refine-routes-list-functionality branch from ea39413 to b698744 Compare December 10, 2024 10:17
settermjd added a commit to settermjd/mezzio-tooling that referenced this pull request Dec 10, 2024
As per @gsteel's recommendation in
mezzio#58 (comment),
this change changes how the class is registered with the DI container.

Signed-off-by: Matthew Setter <[email protected]>
@froschdesign
Copy link
Member

@settermjd
Please check the conflicts.
Thanks in advance! 👍🏻

settermjd and others added 13 commits December 18, 2024 09:59
Previously, RoutesFilter was taking an array with the filter options
(i.e., method, path, name, etc) but, as @gsteel noted, there was a
perfectly good RouteFilterOptions class available. I don't know why I
developed it, but overlooked it. This change refactors RoutesFilter to
use it instead, then goes one step further and introduces a
RouteFilterOptionsInterface so that, when no filter options are
available, an EmptyRouteFilterOptions can be supplied, hopefully making
the code easier to intuit and maintain.

Signed-off-by: Matthew Setter <[email protected]>
This change stores the filter options as member variables so that
they're more readily reusable and so that it should be less likely to
make a mistake with using them at some point in the future.

Signed-off-by: Matthew Setter <[email protected]>
This change removes the class' default options, replacing them with
unions of non-empty-string|null. In doing so it's explicit that
null means the option is unset and should be ignored.

This change was motivated by @gsteel's comment
mezzio#52 (comment).
I believe that I've implemented it correctly.

Signed-off-by: Matthew Setter <[email protected]>
This is to ensure that any possible type of error is caught and handled.

Signed-off-by: Matthew Setter <[email protected]>
I'm not sure why I didn't do this originally. Perhaps I was
experimenting to learn how routing works in Mezzio more deeply.
Regardless, this change refactors ListRoutesCommand to take a
RouteCollector object instead of a ContainerInterface and from that
ContainerInterface fetching a RouteCollector.

Signed-off-by: Matthew Setter <[email protected]>
Signed-off-by: Matthew Setter <[email protected]>
This change refactors ListRoutesCommandFactory to retrieve a
ConfigLoaderInterface object from the application's DiC instead of
instantiating a RoutesFileConfigLoader object directly. The main reason
for this is to provide flexibility to the user, should they have the
need to retrieve routes in a different way than the default
implementation, which searches in config/routes.php and in any routes
registered through ConfigProvider classes.

Signed-off-by: Matthew Setter <[email protected]>
This change provides a default implementation for retrieving an
application's routes configuration. It uses the stock approach of
searching for routes in config/routes.php and any registered with the
Application object in any of the loaded ConfigProvider classes. It also
provides a simplistic example for building a custom implementation,
should it be required.

Signed-off-by: Matthew Setter <[email protected]>
This change updates the packages ConfigProvider class to register a
ConfigLoaderInterface service with the DI container provided by
DefaultRoutesConfigLoaderFactory. If a user needs to override this
implementation, they can do so as per the additional documentation in
the package's README.

Signed-off-by: Matthew Setter <[email protected]>
Co-authored-by: George Steel <[email protected]>
Signed-off-by: Matthew Setter <[email protected]>
Co-authored-by: George Steel <[email protected]>
Signed-off-by: Matthew Setter <[email protected]>
Thanks to @froschdesign for pointing this out in
mezzio#58 (comment).

Signed-off-by: Matthew Setter <[email protected]>
As pointed out by @gsteel, this implementation is not required.

Signed-off-by: Matthew Setter <[email protected]>
As @gsteel pointed out, this was provided in the constructor but
overwritten later, making it redundant.

Signed-off-by: Matthew Setter <[email protected]>
As per @gsteel's recommendation in
mezzio#58 (comment),
this change changes how the class is registered with the DI container.

Signed-off-by: Matthew Setter <[email protected]>
As pointed out by @gsteel, the method is not used outside of tests, so
can be removed.

Signed-off-by: Matthew Setter <[email protected]>
This change fixes a small bug in the class constructor that overrides
the work sanitising the method data provided in the call to array_map.

Signed-off-by: Matthew Setter <[email protected]>
Thanks for feedback from @gsteel, this change introduces Symfony's
CommandTester in ListRoutesCommandTest to simplify the tests and to make
them (far) more maintainable.

Signed-off-by: Matthew Setter <[email protected]>
@settermjd settermjd force-pushed the refine-routes-list-functionality branch from 46fb50b to 633d12f Compare December 19, 2024 04:23
This change handles if null methods or Route::HTTP_METHOD_ANY is listed
as a method.

Signed-off-by: Matthew Setter <[email protected]>
This change uses a RouteFilterOptions object instead of an array, as the
class is much more specific than an array and was already available in
the source tree.

Signed-off-by: Matthew Setter <[email protected]>
@settermjd settermjd force-pushed the refine-routes-list-functionality branch from 633d12f to d7380b5 Compare December 19, 2024 04:25
@settermjd
Copy link
Contributor Author

@gsteel @froschdesign please let me know if changes are still required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants